-
Notifications
You must be signed in to change notification settings - Fork 3
feat: inner agentic loop for openai responses requests (blocking only) #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: graphite-base/127
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
…o bridge (#102) The actions of determining whether to use Anthropic's streaming APIs and ensuring a uniform message payload format used to unmarshal and remarshal the same payload twice. This PR ensures we unmarshal once, do both, and then remarshal once.
| i.reqPayload, err = sjson.SetBytes(i.reqPayload, "input", i.req.Input) | ||
| if err != nil { | ||
| i.logger.Error(ctx, "failure to marshal new input in inner agentic loop", slog.Error(err)) | ||
| // TODO: what should be returned under this condition? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think simply returning internal server error is ok.
f6526ad to
101b739
Compare
| i.prepareRequestForAgenticLoop(response) | ||
| i.req.Input.OfInputItemList = append(i.req.Input.OfInputItemList, results...) | ||
|
|
||
| i.reqPayload, err = sjson.SetBytes(i.reqPayload, "input", i.req.Input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm scared re-marshaling whole input array could cause re-encoding issues
sjson allows to append items to existing array by using -1 index value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but unfortunately we'll need to remarshal at least some of the time since Input starts out as a single string (the prompt) and then changes to a list thanks for the fuckery of this API.
Gonna keep it as-is for now, but we may have to get clever later.
Added a note.
| i.reqPayload, err = sjson.SetBytes(i.reqPayload, "input", i.req.Input) | ||
| if err != nil { | ||
| i.logger.Error(ctx, "failure to marshal new input in inner agentic loop", slog.Error(err)) | ||
| // TODO: what should be returned under this condition? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think simply returning internal server error is ok.
| "github.com/tidwall/sjson" | ||
| ) | ||
|
|
||
| func (i *responsesInterceptionBase) injectTools() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be tested in base_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment.
|
|
||
| // prepareRequestForAgenticLoop prepares the request by setting the output of the given | ||
| // response as input to the next request, in order for the tool call result(s) to make function correctly. | ||
| func (i *BlockingResponsesInterceptor) prepareRequestForAgenticLoop(response *responses.Response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be unit tested, also other functions added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration tests already test these; coverage is pretty high.
Later we could add some unit tests here but I think coverage is pragmatic for now.
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
101b739 to
d44ad71
Compare

TODO